[FEAT] logging refactors 1/n #4439

tchaton · 2020-10-30T10:05:00Z

What does this PR do?

This PR start introducing new changes for refactoring logging.

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2020-10-30T10:05:11Z

Hello @tchaton! Thanks for updating this PR.

In the file pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py:

Line 62:53: E231 missing whitespace after ':'

In the file tests/trainer/logging/test_logger_connector.py:

Line 86:121: E501 line too long (133 > 120 characters)
Line 87:121: E501 line too long (133 > 120 characters)
Line 181:121: E501 line too long (123 > 120 characters)
Line 244:121: E501 line too long (126 > 120 characters)
Line 248:121: E501 line too long (131 > 120 characters)

Comment last updated at 2020-11-02 18:47:07 UTC

justusschock

I like this really much. It makes everything much easier to follow!

I left some comments here.
Once this is finished, could you also go over the files as well and add docstrings for all the functions and maybe some type hints as well? This would enhance the readability even further :)

pytorch_lightning/core/lightning.py

pytorch_lightning/trainer/connectors/logger_connector/__init__.py

pytorch_lightning/trainer/logging.py

pytorch_lightning/trainer/training_loop.py

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

codecov · 2020-10-30T12:16:12Z

Codecov Report

Merging #4439 into master will decrease coverage by 1%.
The diff coverage is 84%.

@@           Coverage Diff           @@
##           master   #4439    +/-   ##
=======================================
- Coverage      93%     92%    -1%     
=======================================
  Files         113     116     +3     
  Lines        8198    8700   +502     
=======================================
+ Hits         7633    8047   +414     
- Misses        565     653    +88

williamFalcon · 2020-10-30T13:04:33Z

mind adding it to the title so we can track that this is partial?

ref: logging refactors 1/n

SeanNaren

Overall this is incredibly neat @tchaton!

pytorch_lightning/core/lightning.py

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>

…rchLightning/pytorch-lightning into feat/epoch_loop_result_objs

justusschock

Again, I really like this.

Some minor comments, but nothing major!

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

…sult_store.py Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

…rchLightning/pytorch-lightning into feat/epoch_loop_result_objs

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py

SeanNaren

It's a lot of code at once, however I've reviewed this multiple times and it looks clear and consistent. Let's get this over the finish line :)

…sult_store.py Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>

…rchLightning/pytorch-lightning into feat/epoch_loop_result_objs

teddykoker

LGTM, awesome work Thomas :)

* introducing new logging object * typo * typo * Update pytorch_lightning/trainer/logging.py Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> * Update pytorch_lightning/trainer/logging.py Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> * update on comments * update on comments * add more doctstring * Update pytorch_lightning/core/lightning.py Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> * resolve on comments * solve pyright * Update pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> * update on comments * Update pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> * update on comments Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>

introducing new logging object

e349141

tchaton requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, teddykoker and williamFalcon as code owners October 30, 2020 10:05

tchaton changed the title ~~[FEAT] Add new object for logging. No activated yet.~~ [FEAT] Add new object for logging. Not activated yet. Oct 30, 2020

typo

8417a15

tchaton changed the title ~~[FEAT] Add new object for logging. Not activated yet.~~ [FEAT] Refactor Logging. Not activated yet. Oct 30, 2020

tchaton added 2 commits October 30, 2020 10:40

typo

e21b249

Merge branch 'master' into feat/epoch_loop_result_objs

edd3818

justusschock reviewed Oct 30, 2020

View reviewed changes

tchaton and others added 5 commits October 30, 2020 11:20

Update pytorch_lightning/trainer/logging.py

f9022d7

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

Update pytorch_lightning/trainer/logging.py

f7fd584

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

update on comments

de910e1

update on comments

eaf349b

add more doctstring

b81aecf

edenlightning added the refactor label Oct 30, 2020

edenlightning added this to the 1.0.x milestone Oct 30, 2020

tchaton changed the title ~~[FEAT] Refactor Logging. Not activated yet.~~ [FEAT] logging refactors 1/n Oct 30, 2020

SeanNaren reviewed Oct 30, 2020

View reviewed changes

tchaton and others added 3 commits October 30, 2020 17:44

Merge branch 'master' into feat/epoch_loop_result_objs

e80f2e8

Update pytorch_lightning/core/lightning.py

694ded2

Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>

resolve on comments

cab95b8

tchaton added 4 commits November 2, 2020 07:55

Merge branch 'master' into feat/epoch_loop_result_objs

4190cf5

solve pyright

0fbe5ea

Merge branch 'feat/epoch_loop_result_objs' of https://github.com/PyTo…

46b76f6

…rchLightning/pytorch-lightning into feat/epoch_loop_result_objs

Merge branch 'master' into feat/epoch_loop_result_objs

1c47c8a

justusschock approved these changes Nov 2, 2020

View reviewed changes

tchaton and others added 4 commits November 2, 2020 10:06

Update pytorch_lightning/trainer/connectors/logger_connector/epoch_re…

49a491a

…sult_store.py Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

update on comments

ac09913

Merge branch 'feat/epoch_loop_result_objs' of https://github.com/PyTo…

f8f06cf

…rchLightning/pytorch-lightning into feat/epoch_loop_result_objs

Merge branch 'master' into feat/epoch_loop_result_objs

a9f6e56

SeanNaren mentioned this pull request Nov 2, 2020

loss=None and no logs when automatic_optimization=False #4204

Closed

SeanNaren reviewed Nov 2, 2020

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/logger_connector/epoch_result_store.py Outdated Show resolved Hide resolved

SeanNaren approved these changes Nov 2, 2020

View reviewed changes

tchaton and others added 3 commits November 2, 2020 13:36

Update pytorch_lightning/trainer/connectors/logger_connector/epoch_re…

e604313

…sult_store.py Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>

update on comments

14f92b0

Merge branch 'feat/epoch_loop_result_objs' of https://github.com/PyTo…

92c8a3f

…rchLightning/pytorch-lightning into feat/epoch_loop_result_objs

tchaton added the ready PRs ready to be merged label Nov 2, 2020

SeanNaren and others added 3 commits November 2, 2020 14:06

Merge branch 'master' into feat/epoch_loop_result_objs

55c7a2c

Merge branch 'master' into feat/epoch_loop_result_objs

05baa28

Merge branch 'master' into feat/epoch_loop_result_objs

518929d

teddykoker approved these changes Nov 2, 2020

View reviewed changes

Merge branch 'master' into feat/epoch_loop_result_objs

80e825c

tchaton merged commit ac3f739 into master Nov 2, 2020

tchaton deleted the feat/epoch_loop_result_objs branch November 2, 2020 20:51

Borda mentioned this pull request Nov 3, 2020

feature: Add logging test / val callback functions #4351

Closed

8 tasks

Borda modified the milestones: 1.0.x, 1.1 Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] logging refactors 1/n #4439

[FEAT] logging refactors 1/n #4439

tchaton commented Oct 30, 2020 •

edited

Loading

pep8speaks commented Oct 30, 2020 •

edited

Loading

justusschock left a comment

codecov bot commented Oct 30, 2020 •

edited

Loading

williamFalcon commented Oct 30, 2020

SeanNaren left a comment

justusschock left a comment

SeanNaren left a comment

teddykoker left a comment

[FEAT] logging refactors 1/n #4439

[FEAT] logging refactors 1/n #4439

Conversation

tchaton commented Oct 30, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Oct 30, 2020 • edited Loading

Comment last updated at 2020-11-02 18:47:07 UTC

justusschock left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 30, 2020 • edited Loading

Codecov Report

williamFalcon commented Oct 30, 2020

SeanNaren left a comment

Choose a reason for hiding this comment

justusschock left a comment

Choose a reason for hiding this comment

SeanNaren left a comment

Choose a reason for hiding this comment

teddykoker left a comment

Choose a reason for hiding this comment

tchaton commented Oct 30, 2020 •

edited

Loading

pep8speaks commented Oct 30, 2020 •

edited

Loading

codecov bot commented Oct 30, 2020 •

edited

Loading